-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ValueSet self reference #1503
ValueSet self reference #1503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though the Jira issue didn't link to it, I do think this resolves #1487. If you add "Fixes #1487" to the PR description, it should automatically close the issue once this PR is merged. Then again, maybe me typing this in the comment will do that too. I guess we'll see! (Edit: Present-time-Julia here to say that this comment did not link the issue to the PR, so you can add it to the description to link it and have it automatically close.)
I also left some comments inline with questions about how the behavior here works. They probably are worth discussing with the team (unless these questions have been answered last week and I just missed it!).
src/export/ValueSetExporter.ts
Outdated
composeElement.valueSet = composeElement.valueSet.filter(vs => { | ||
if (vs == valueSet.url) { | ||
logger.error( | ||
`Value set with id ${valueSet.id} has component rule with self referencing value set (by id, value set name, or url). Skipping rule.` | ||
); | ||
} | ||
return vs != valueSet.url; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a question of what we want the desired output to be. What you have here might be totally correct, but I just want to double check.
Now that we're filtering the composeElement.valueSet
, there's a chance that we're left with no items in composeElement.valueSet
. Then on line 187, we push the composeElement
object onto valueset.compose.include
no matter what the value of composeElement
is. Since we could have filtered out all the value sets, we might be pushing on a compose element that doesn't have any value sets, which then gets converted to a null
value within an array when we create the FHIR resources. Since I might have explained that poorly, here's an example of what I'm trying to say:
If you have the following value set (and another value set defined for OtherVS
)
ValueSet: MaxVS
Id: max-vs
* include codes from valueset MaxVS
* include codes from valueset OtherVS
the first rule will filter out the MaxVS
value set because it is self-reference. That then results in the following compose
element to be created:
"compose": {
"include": [
null,
{
"valueSet": [
"http://example.org/ValueSet/other-vs"
]
}
]
}
Since we're skipping the rule and telling authors about it, I wasn't expecting a null
entry where the filtered value set was removed. Is that what we were expecting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking through this in person, we decided that the expected outcome of that FSH snippet would be
"compose": {
"include": [
{
"valueSet": [
"http://example.org/ValueSet/other-vs"
]
}
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is "a real pain in the neck" to get rid of the null
, don't worry too much because we logged an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into this further with the debugger, I do not see null values added to the compose.include array. The test I added does not show resulting null values in its array either (If it did, there would be 3 null values since we show the 3 different self references in that example).
I see compose.include = null when given only self-referencing values in the value set (but null is not added to the compose.include array) - in this case there is no array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I think the case I was describing is a little different from what you described. I agree that the test shows we skip over the self-referenced value set and no null
ends up in the compose.include.valueset
array it constructs. I think the other example you described where there is no array at all is happens when the only rule on the value set has only a reference to itself (I might be wrong though, so let me know if I misunderstood this case).
The case that I was trying to ask about is if you have two include rules and one has a valid reference to a value set and the other has a self-referenced value set. In that case, because the compose.include
array is being created, I'm seeing a null
value entered into that array for the self-referenced value set. If you use the example FSH I included and run it on this branch and look at the generated ValueSet for MaxVS, I think you should see the null
value. Then again, maybe I'm doing something wrong, so let me know if you can't reproduce that.
Also, we did decide that if "it is a 'real pain in the neck' to get rid of the null
", then we just would leave it. So if this is reaching pain in the neck status, let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with the recent changes. Let me know if this still does not cover this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making these updates. I have a comment about the conditional statement you added in the exporter.
src/export/ValueSetExporter.ts
Outdated
valueSet.compose.include.push(composeElement); | ||
if (composeElement.valueSet?.length != 0) { | ||
valueSet.compose.include.push(composeElement); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this condition might be a little too restrictive. Consider the following FSH:
ValueSet: SomeVS
* include codes from system http://example.org/SomeCS and valueset SomeVS
In this case, the composeElement.valueSet
will be an empty array, so composeElement
will not be added to the value set. But, composeElement.system
has a value, so I think that composeElement
should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and covers the case I had mentioned! I left one little comment, but if you disagree with it, just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks for making all those updates! This seems like it will be very helpful to catch these sneaky mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates! Approved!
Description: This PR addresses when a component rule on a ValueSet attempts to self-reference by id, ValueSet name, or url. For this case, it logs an error and removes the self reference.
Testing Instructions: Verify with unit tests.
Related Issue: Fixes #1487